Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: catch unhandled error when adding or refreshing oauth apps #35089

Merged
merged 4 commits into from
Feb 4, 2025
Merged

fix: catch unhandled error when adding or refreshing oauth apps #35089

merged 4 commits into from
Feb 4, 2025
+8 −2

Conversation

julio-rocketchat
Copy link
Member

@julio-rocketchat julio-rocketchat commented Feb 2, 2025 •
edited by jira bot
Loading

Proposed changes (including videos or screenshots)

When adding a new OAuth app or refreshing OAuth apps, sometimes we get caught in an unhandled promise rejection that ends up crashing the server. The error seems to happen because of some sort of race condition - sometimes the application tries to fetch an ID from the MongoDB, but it's not there yet and it will throw the unhandled promise.

@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR) === UnHandledPromiseRejection ===
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR) Error: Could not find element with id -67a0d69eb3ed3f15eb312d82 to change
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at SessionCollectionView.changed (packages/ddp-server/livedata_server.js:266:13)
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at Session.changed (packages/ddp-server/livedata_server.js:490:12)
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at Subscription.changed (packages/ddp-server/livedata_server.js:1416:19)
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at Object.changed (packages/mongo/collection/collection.js:110:17)
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at server/services/meteor/service.ts:156:7
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at Set.forEach (<anonymous>)
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at EventEmitter.<anonymous> (server/services/meteor/service.ts:155:29)
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at EventEmitter.emit (node:events:518:28)
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at EventEmitter.emit (node:domain:489:12)
@rocket.chat/meteor:dsv: W20250203-15:45:50.613(1)? (STDERR)     at LocalBroker.broadcastLocal (/Users/julio/Downloads/Rocket.Chat/packages/core-services/src/LocalBroker.ts:96:15)
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR)     at LocalBroker.broadcast (/Users/julio/Downloads/Rocket.Chat/packages/core-services/src/LocalBroker.ts:90:8)
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR)     at Api.broadcast (/Users/julio/Downloads/Rocket.Chat/packages/core-services/src/lib/Api.ts:49:22)
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR)     at app/lib/server/lib/notifyListener.ts:170:12
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR)     at app/lib/server/lib/notifyListener.ts:187:8
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR)     at processTicksAndRejections (node:internal/process/task_queues:105:5)
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR) ---------------------------------
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR) Errors like this can cause oplog processing errors.
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR) Setting EXIT_UNHANDLEDPROMISEREJECTION will cause the process to exit allowing your service to automatically restart the process
@rocket.chat/meteor:dsv: W20250203-15:45:50.614(1)? (STDERR) Future node.js versions will automatically exit the process
@rocket.chat/meteor:dsv: W20250203-15:45:50.617(1)? (STDERR) =================================
@rocket.chat/meteor:dsv: => Exited with code: 1
@rocket.chat/meteor:dsv: => Your application is crashing. Waiting for file change.

I've noticed that even though it crashes the server, the OAuth app is still added to the database and refreshing OAuth apps also work just fine. This PR adds try/catch to prevent the server from crashing when that happens.

Issue(s)

SB-762

Steps to test or reproduce

Run the latest version of RocketChat and try to add an OAuth app or refresh OAuth apps.

Further comments

N/A

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 2, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 7aa49c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 35 packages
Name Type
@rocket.chat/meteor Minor
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Major
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-client Major
@rocket.chat/ui-video-conf Major
@rocket.chat/ui-voip Major
@rocket.chat/web-ui-registration Major
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 2, 2025 •
edited
Loading

PR Preview Action v1.6.0

🚀 View preview at
https://RocketChat.github.io/Rocket.Chat/pr-preview/pr-35089/

Built to branch gh-pages at 2025-02-03 21:49 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@codecov Codecov
Copy link

codecov bot commented Feb 2, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.22%. Comparing base (3fda478) to head (7aa49c3).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #35089   +/-   ##
========================================
  Coverage    59.22%   59.22%           
========================================
  Files         2824     2824           
  Lines        68071    68069    -2     
  Branches     15153    15151    -2     
========================================
- Hits         40315    40314    -1     
  Misses       24924    24924           
+ Partials      2832     2831    -1
Flag Coverage Δ
unit 75.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@julio-rocketchat julio-rocketchat marked this pull request as ready for review February 3, 2025 14:49
@julio-rocketchat julio-rocketchat requested a review from a team as a code owner February 3, 2025 14:49
@dionisio-bot dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Feb 3, 2025 •
edited
Loading

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@julio-rocketchat julio-rocketchat added this to the 7.4.0 milestone Feb 3, 2025
KevLehman
KevLehman previously approved these changes Feb 3, 2025
View reviewed changes
@KevLehman
Copy link
Member

Wonder if this is more of a chore. But if you think it's a fix please add a changeset

@julio-rocketchat
Copy link
Member Author

Wonder if this is more of a chore. But if you think it's a fix please add a changeset

Will do. Grazie mille

@julio-rocketchat julio-rocketchat added the stat: QA assured Means it has been tested and approved by a company insider label Feb 3, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Feb 3, 2025
@kodiakhq kodiakhq bot merged commit a36d02f into develop Feb 4, 2025
48 checks passed
@kodiakhq kodiakhq bot deleted the fix-catch-error-when-adding-oauth-app branch February 4, 2025 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevLehman KevLehman KevLehman approved these changes

@sampaiodiego sampaiodiego sampaiodiego approved these changes

Assignees
No one assigned
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Milestone
7.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@julio-rocketchat @KevLehman @sampaiodiego